Skip to content

GH-36831: [C++] DictionaryArray support for MinMax Function#37100

Open
R-JunmingChen wants to merge 67 commits into
apache:mainfrom
R-JunmingChen:ARROW-36831
Open

GH-36831: [C++] DictionaryArray support for MinMax Function#37100
R-JunmingChen wants to merge 67 commits into
apache:mainfrom
R-JunmingChen:ARROW-36831

Conversation

@R-JunmingChen

@R-JunmingChen R-JunmingChen commented Aug 10, 2023 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

As #36831 requires, a MinMax kernel for DictionaryArray is supported.

What changes are included in this PR?

Add a DictionaryType kernel for MinMax

Are these changes tested?

Yes, A test is added in aggregate_test.cc

Are there any user-facing changes?

No.

@github-actions github-actions Bot added the awaiting review Awaiting review label Aug 10, 2023
@github-actions

github-actions Bot commented Aug 10, 2023

Copy link
Copy Markdown

⚠️ GitHub issue #36831 has been automatically assigned in GitHub to PR creator.

@R-JunmingChen

R-JunmingChen commented Aug 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi, reviewers, I need a guidance.

The resolved output type for DictionaryType MinMax Kernel is DictionaryType now. But I think the MinMax output type should be the DataType of the values of the DictionaryType. So, in my implementation, I use the DataType of the values of the DictionaryType as output type, which raise an error like the following img currently.
image

Can we have a correct resolved output type? and where are the codes should be changed? It should be mentioned that we can only obtain the output type in the runtime instead of compile time.

@R-JunmingChen R-JunmingChen marked this pull request as ready for review August 10, 2023 15:49
@js8544

js8544 commented Aug 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

The AddMinMaxKernel function declares the signature with output MinMaxType as output, you should modify this function if you want it to output its value type instead of the dictionary itself.

Be aware that min and max also reuse the min_max kernels, so you should take a look at AddMinOrMaxAggKernel too.

mapleFU
mapleFU reviewed Aug 11, 2023
View reviewed changes
Comment thread cpp/src/arrow/compute/kernels/aggregate_basic.cc Outdated
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 11, 2023
@R-JunmingChen R-JunmingChen requested a review from mapleFU August 14, 2023 12:24
@R-JunmingChen R-JunmingChen mentioned this pull request Oct 25, 2023
Closed
@R-JunmingChen R-JunmingChen requested a review from pitrou November 30, 2023 01:28
@R-JunmingChen

R-JunmingChen commented Nov 30, 2023

Copy link
Copy Markdown
Contributor Author

Hi, @pitrou, @js8544. please review this PR when you have time.

@R-JunmingChen

R-JunmingChen commented Dec 18, 2023

Copy link
Copy Markdown
Contributor Author

hi, @bkietz, could you please review this PR when you have time?

bkietz
bkietz requested changes Feb 1, 2024
View reviewed changes

@bkietz bkietz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please rebase and correct the CI failure

One item for potential follow up:

Comment thread cpp/src/arrow/compute/kernels/aggregate_basic_internal.h
Comment thread cpp/src/arrow/compute/kernels/aggregate_basic_internal.h Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 1, 2024
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 4, 2024
@R-JunmingChen R-JunmingChen requested a review from bkietz February 4, 2024 12:00
@bkietz

bkietz commented Mar 27, 2024

Copy link
Copy Markdown
Member

@R-JunmingChen sorry for letting this slip off my radar! Could you rebase please?

@R-JunmingChen

R-JunmingChen commented Apr 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi, @bkietz, I have merged the latest main branch. The only one CI error seems not related

@R-JunmingChen

R-JunmingChen commented Apr 3, 2024

Copy link
Copy Markdown
Contributor Author

ping @bkietz

@R-JunmingChen

R-JunmingChen commented Apr 15, 2024

Copy link
Copy Markdown
Contributor Author

ping @bkietz again

@mapleFU

mapleFU commented Apr 23, 2024

Copy link
Copy Markdown
Member

@pitrou Would you mind check this?

@github-actions

github-actions Bot commented Nov 18, 2025

Copy link
Copy Markdown

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

@github-actions github-actions Bot added the Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise label Nov 18, 2025
@thisisnic thisisnic added Status: needs champion High impact issues which aren't being worked on but require a volunteer to move the task forward. and removed Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise labels Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mapleFU mapleFU mapleFU approved these changes
@js8544 js8544 Awaiting requested review from js8544
@pitrou pitrou Awaiting requested review from pitrou pitrou is a code owner
@bkietz bkietz Awaiting requested review from bkietz

Assignees

No one assigned

Labels

awaiting change review Awaiting change review Component: C++ Status: needs champion High impact issues which aren't being worked on but require a volunteer to move the task forward.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Dictionary support for ordering-based compute functions.

6 participants

@R-JunmingChen @js8544 @mapleFU @bkietz @pitrou @thisisnic